-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Share code between cask token and formula name audits. #17562
base: master
Are you sure you want to change the base?
Conversation
b7a14ac
to
1f5a2c2
Compare
# frozen_string_literal: true | ||
|
||
module Homebrew | ||
class TokenAuditor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class TokenAuditor | |
class NameAuditor |
Thoughts? Token is cask-specific so I think maybe "name" is clearer in this case. NBD though either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name
has a different meaning for casks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but also token
has no meaning for formulae. When I'm speaking about what to call a formula or cask, I use the word "name" and mean Formula#name
and Cask#token
.
I'll leave it up to other people for their thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class TokenAuditor | |
class FormulaNameCaskTokenAuditor |
or something.
I agree TokenAuditor
doesn't make sense if it can operate on Formula because they don't have tokens.
# errors << "underscores" if token.include?("_") | ||
# errors << "plus symbols" if token.include?("+") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe accept the formula or cask itself in the initializer and then check these only if it's a cask?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we allow _
and +
for formula names, I don't really see a good reason to disallow it for casks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly at the moment, but the comments should be removed if this is the plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll remove the comments before merging. We should check if casks should be renamed/aliased (plus
-> +
).
I think it's nicer to maintain consistency between them both, making/keeping them both stricter. |
1f5a2c2
to
44fd8ad
Compare
44fd8ad
to
e2655a3
Compare
Can you clarify what you mean here? |
I'm not sure how to clarify, really! I'm just disagreeing with the sentence here that relaxing the token audit so that it's not consistent with the formula name audit seems like introducing undesirable inconsistency. |
The cask token audit is relaxed a bit while the formula name is made more strict, so the end result is they are consistent. |
Hah, ok, sorry for my misunderstanding then 👍🏻 |
name = formula.name | ||
|
||
problem "Formula name '#{name}' must not contain uppercase letters." if name != name.downcase | ||
token_auditor = Homebrew::TokenAuditor.new(name) | ||
unless (errors = token_auditor.errors).empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless (errors = token_auditor.errors).empty? | |
if (errors = token_auditor.errors).any? |
or
unless (errors = token_auditor.errors).empty? | |
if (errors = token_auditor.presence) |
# frozen_string_literal: true | ||
|
||
module Homebrew | ||
class TokenAuditor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class TokenAuditor | |
class FormulaNameCaskTokenAuditor |
or something.
I agree TokenAuditor
doesn't make sense if it can operate on Formula because they don't have tokens.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This relaxes the casks token audit and makes the formula name audit more strict.